Allow feature builtins to overwrite existing builtins by CriesofCarrots · Pull Request #13403 · solana-labs/solana

CriesofCarrots · 2020-11-04T22:31:48Z

Problem

Right now, there is no easy way to redeploy a native program (to fix issues, eg). One appealing approach would be to deploy the updated program using feature_builtins, but this requires a way to overwrite an active builtin.

Summary of Changes

Add a method to enable overwriting existing builtins
Use this method in ensure_feature_builtins to allow a feature_builtin to replace an existing builtin

Needed for #13394

Pull request has been modified.

ryoqun · 2020-11-05T06:23:21Z

            assert!(
                !self.is_frozen(),
                "Can't change frozen bank by adding not-existing new native program ({}, {}). \
                Maybe, inconsistent program activation is detected on snapshot restore?",
                name,
                program_id
            );


Could you move this assertion to reside immediately before store_account? This assertion is tightly associated to store_account. We can safely assert!(!self.is_frozen()) even if must_replace is true. In general, we must not store_account once frozen in every conceivable situation. But that kind of protection isn't easy to add inside store_account itself, unfortunately. So, we're manually doing this because these activations deal with primitive arrangements with potentinal misbehavior of wracking frozen (snapshot) banks incorrectly.

So, the change hunk could be changed to look like this:

if must_replace { assert!(already_genuine_program_exists, ...) } else if already_genuine_program_exists { return; } assert!(!self.is_frozen(), ...); self.store_account(...);

@ryoqun Moving this assert breaks a bunch of snapshot tests once a NewVersion builtin is added. What do you recommend?

@CriesofCarrots Blame me, I completely forget that this fn must retain idempotent of AccountsDB; so more subtle adjustment was needed: 2a9ff42 Hope this commit explains well. If still not sure, please say so. I'll update the in-source comment.

Sorry for confusing you because of my unreadable code. ;)

ryoqun · 2020-11-05T06:27:07Z

Thanks for working on this! Good progress. In addition to NewProgram, I think this added NewVersion is decent usecase as well.

ryoqun

lgtm with a nit

Thanks for improving the native program initialization mechanism!

Pull request has been modified.

codecov · 2020-11-05T07:57:07Z

Codecov Report

Merging #13403 into master will decrease coverage by 0.0%.
The diff coverage is 90.9%.

@@            Coverage Diff            @@
##           master   #13403     +/-   ##
=========================================
- Coverage    81.9%    81.9%   -0.1%     
=========================================
  Files         376      376             
  Lines       88308    88356     +48     
=========================================
+ Hits        72345    72382     +37     
- Misses      15963    15974     +11

mergify · 2020-11-05T14:17:09Z

automerge label removed due to a CI failure

* Allow feature builtins to overwrite existing builtins * Add feature_builtin ActivationType * Correctly retain idempotent for replacing case * Fix test Co-authored-by: Ryo Onodera <ryoqun@gmail.com> (cherry picked from commit bc62313) # Conflicts: # ledger/src/builtins.rs # runtime/src/bank.rs

* Allow feature builtins to overwrite existing builtins * Add feature_builtin ActivationType * Correctly retain idempotent for replacing case * Fix test Co-authored-by: Ryo Onodera <ryoqun@gmail.com> (cherry picked from commit bc62313)

CriesofCarrots · 2020-11-05T16:25:14Z

+        // When replacing native_program, name must change to disambiguate from repeated
+        // invocations.


I think we should put a comment about this in builtins.rs, since it's critical to know when constructing the name

I'll sneak that into #13394

* Allow feature builtins to overwrite existing builtins * Add feature_builtin ActivationType * Correctly retain idempotent for replacing case * Fix test Co-authored-by: Ryo Onodera <ryoqun@gmail.com> (cherry picked from commit bc62313) Co-authored-by: Tyera Eulberg <teulberg@gmail.com>

…3419) * Allow feature builtins to overwrite existing builtins (#13403) * Allow feature builtins to overwrite existing builtins * Add feature_builtin ActivationType * Correctly retain idempotent for replacing case * Fix test Co-authored-by: Ryo Onodera <ryoqun@gmail.com> (cherry picked from commit bc62313) # Conflicts: # ledger/src/builtins.rs # runtime/src/bank.rs * Fix conflicts Co-authored-by: Tyera Eulberg <teulberg@gmail.com> Co-authored-by: Tyera Eulberg <tyera@solana.com>

ryoqun · 2020-12-04T09:22:26Z

+        let mut existing_genuine_program = self.get_account(&program_id);
+        if let Some(account) = &mut existing_genuine_program {
+            if !native_loader::check_id(&account.owner) {
                // malicious account is pre-occupying at program_id
                // forcibly burn and purge it


well this logic changes introduced a regression where we could be tricked into skipping to create native program because existing_genuine_program can be set to a bad account and not reset and we enters this match arm returning from this fn: https://github.com/solana-labs/solana/pull/13403/files#diff-ed47b4a0198313377e091bb3957bbbc63d937805426d1b2b6de39d0a50d32a0cR1760

Fixed and test is imrproved at #13884.

ryoqun · 2020-12-04T09:26:07Z

+        );
+
+        // Add a bogus executable native account, which will be loaded and ignored.
+        let account = native_loader::create_loadable_account(name);


Also, when replacing, we must consider the balance of existing program account otherwise we would cause capitalization mismatch if the replaced native program isn't 1 lamports.

Fixed and test added at #13884.

Allow feature builtins to overwrite existing builtins

9589503

CriesofCarrots added v1.3 labels Nov 4, 2020

CriesofCarrots requested review from mvines and ryoqun November 4, 2020 22:31

CriesofCarrots mentioned this pull request Nov 4, 2020

Feature-gate stake program #13394

Merged

mvines previously approved these changes Nov 4, 2020

View reviewed changes

CriesofCarrots force-pushed the feature-builtins-overwrite branch 2 times, most recently from 0b7ac24 to 3957cf8 Compare November 5, 2020 06:02

ryoqun reviewed Nov 5, 2020

View reviewed changes

Comment thread runtime/src/builtins.rs

CriesofCarrots force-pushed the feature-builtins-overwrite branch from 3957cf8 to b896705 Compare November 5, 2020 06:34

ryoqun reviewed Nov 5, 2020

View reviewed changes

Comment thread runtime/src/bank.rs Outdated

ryoqun previously approved these changes Nov 5, 2020

View reviewed changes

Add feature_builtin ActivationType

ec49a46

CriesofCarrots force-pushed the feature-builtins-overwrite branch from b896705 to ec49a46 Compare November 5, 2020 06:50

Correctly retain idempotent for replacing case

2a9ff42

ryoqun added the automerge Merge this Pull Request automatically once CI passes label Nov 5, 2020

mergify Bot removed the automerge Merge this Pull Request automatically once CI passes label Nov 5, 2020

Fix test

36e2e6d

ryoqun added the automerge Merge this Pull Request automatically once CI passes label Nov 5, 2020

mergify Bot merged commit bc62313 into solana-labs:master Nov 5, 2020

mergify Bot mentioned this pull request Nov 5, 2020

Allow feature builtins to overwrite existing builtins (bp #13403) #13419

Merged

mergify Bot mentioned this pull request Nov 5, 2020

Allow feature builtins to overwrite existing builtins (bp #13403) #13420

Merged

CriesofCarrots commented Nov 5, 2020

View reviewed changes

CriesofCarrots deleted the feature-builtins-overwrite branch November 18, 2020 00:26

ryoqun mentioned this pull request Dec 4, 2020

Simple cap by including sysvars and native programs #13884

Merged

ryoqun reviewed Dec 4, 2020

View reviewed changes

ryoqun mentioned this pull request Dec 10, 2020

Tiny add_native_program bug fixes with cleanups #14042

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow feature builtins to overwrite existing builtins#13403

Allow feature builtins to overwrite existing builtins#13403
mergify[bot] merged 4 commits into
solana-labs:masterfrom
CriesofCarrots:feature-builtins-overwrite

CriesofCarrots commented Nov 4, 2020 •

edited

Loading

Uh oh!

ryoqun Nov 5, 2020

Uh oh!

CriesofCarrots Nov 5, 2020 •

edited

Loading

Uh oh!

ryoqun Nov 5, 2020

Uh oh!

ryoqun commented Nov 5, 2020

Uh oh!

Uh oh!

Uh oh!

ryoqun left a comment

Uh oh!

codecov Bot commented Nov 5, 2020 •

edited

Loading

Uh oh!

mergify Bot commented Nov 5, 2020

Uh oh!

CriesofCarrots Nov 5, 2020

Uh oh!

CriesofCarrots Nov 5, 2020

Uh oh!

ryoqun Dec 4, 2020 •

edited

Loading

Uh oh!

ryoqun Dec 4, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		// When replacing native_program, name must change to disambiguate from repeated
		// invocations.

Conversation

CriesofCarrots commented Nov 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Summary of Changes

Uh oh!

ryoqun Nov 5, 2020

Choose a reason for hiding this comment

Uh oh!

CriesofCarrots Nov 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryoqun Nov 5, 2020

Choose a reason for hiding this comment

Uh oh!

ryoqun commented Nov 5, 2020

Uh oh!

Uh oh!

Uh oh!

ryoqun left a comment

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Nov 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mergify Bot commented Nov 5, 2020

Uh oh!

CriesofCarrots Nov 5, 2020

Choose a reason for hiding this comment

Uh oh!

CriesofCarrots Nov 5, 2020

Choose a reason for hiding this comment

Uh oh!

ryoqun Dec 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryoqun Dec 4, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

CriesofCarrots commented Nov 4, 2020 •

edited

Loading

CriesofCarrots Nov 5, 2020 •

edited

Loading

codecov Bot commented Nov 5, 2020 •

edited

Loading

ryoqun Dec 4, 2020 •

edited

Loading